Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat: function array_group_by #7438

Merged
merged 12 commits into from
May 14, 2023
Merged

Conversation

rumpfc
Copy link
Contributor

@rumpfc rumpfc commented Apr 14, 2023

Description
A function to group data rows by values defined by indexes. The function supports the "dot syntax". Optionally it can include empty values (where empty($value) === true) and puts them under key ''.

The depth of return result equals count($indexes).

Example:

$data = [
    [
        'id'    => 1,
        'item'  => 'ball',
        'color' => 'blue',
    ],
    [
        'id'    => 2,
        'item'  => 'book',
        'color' => 'red',
    ],
    [
        'id'   => 3,
        'item' => 'bird',
        'age'  => 5,
    ],
    [
        'id'    => 4,
        'item'  => 'jeans',
        'color' => 'blue',
    ],
];

$result = array_group_by($data, ['color'], true);

Returns following result

$result = [
    'blue' => [
        [
            'id'    => 1,
            'item'  => 'ball',
            'color' => 'blue',
        ],
        [
            'id'    => 4,
            'item'  => 'jeans',
            'color' => 'blue',
        ],
    ],
    'red' => [
        [
            'id'    => 2,
            'item'  => 'book',
            'color' => 'red',
        ],
    ],
    '' => [
        [
            'id'   => 3,
            'item' => 'bird',
            'age'  => 5,
        ],
    ],
];

Checklist:

  • Securely signed commits
  • Component(s) with PHPDoc blocks, only if necessary or adds value
  • Unit testing, with >80% coverage
  • User guide updated
  • Conforms to style guide

@kenjis kenjis added the enhancement PRs that improve existing functionalities label Apr 15, 2023
system/Helpers/array_helper.php Outdated Show resolved Hide resolved
system/Helpers/array_helper.php Outdated Show resolved Hide resolved
system/Helpers/array_helper.php Outdated Show resolved Hide resolved
@kenjis
Copy link
Member

kenjis commented Apr 15, 2023

Please fix the PHPStan error:

Error: Offset mixed on array{} in isset() does not exist.
 ------ ---------------------------------------------------- 
  Line   system/Helpers/array_helper.php                     
 ------ ---------------------------------------------------- 
  256    Offset mixed on array{} in isset() does not exist.  
 ------ ---------------------------------------------------- 

https://github.com/codeigniter4/CodeIgniter4/actions/runs/4706875155/jobs/8348394968?pr=7438

@kenjis kenjis added the 4.4 label Apr 15, 2023
@rumpfc
Copy link
Contributor Author

rumpfc commented Apr 15, 2023

------ ----------------------------------------------------------------- 
  Line   system/Helpers/array_helper.php                                  
 ------ ----------------------------------------------------------------- 
  260    Call to function array_key_exists() with mixed and array{} will  
         always evaluate to false.                                        
 ------ ----------------------------------------------------------------- 

I tried different assignments, incl. string only, but i still get the same error that the function always evaluates to false. But this function call is important for the code to work. Any suggestions?

@kenjis
Copy link
Member

kenjis commented Apr 15, 2023

@rumpfc
Can you remove the references? It is difficult to understand.

@kenjis
Copy link
Member

kenjis commented Apr 15, 2023

Please run: $ composer cs-fix

@rumpfc
Copy link
Contributor Author

rumpfc commented Apr 15, 2023

@rumpfc Can you remove the references? It is difficult to understand.

The references are absolutely necessary. Each iteration of $indexes goes one level deeper into the nested $result. So $currentLevel assigns itself the reference of the subarray.

I can try to implement a recursive alternative (similar how dot_array_search handles it)

@kenjis
Copy link
Member

kenjis commented Apr 15, 2023

I checked the behavior. It seems you are correct, and the PHPStan error seems to be false positive.

If you cannot rewrite easily or if the rewrite code will be more complicated, I suggest you suppress the PHPStan error.

@kenjis
Copy link
Member

kenjis commented Apr 15, 2023

Can you add test case for dot array syntax?

@rumpfc
Copy link
Contributor Author

rumpfc commented Apr 15, 2023

Can you add test case for dot array syntax?

They are already included. See the final yield of both data providers in ArrayHelperTest.

@kenjis
Copy link
Member

kenjis commented Apr 15, 2023

Oh, I missed that. Thank you.

@rumpfc
Copy link
Contributor Author

rumpfc commented Apr 16, 2023

The recursive approach satisfies both rector and phpstan, but I feel like it requires more memory. TIme performance should be the same.

@kenjis
Copy link
Member

kenjis commented Apr 19, 2023

My quick benchmark on macOS shows recursive is slower, but the memory usage are the same.

memory usage: 114232
recursive: 0.8359
reference: 0.6689

    public function test()
    {
        $iterator = new \CodeIgniter\Debug\Iterator();

        $memory_base = memory_get_usage();

        $iterator->add('array_group_by', static function () {
            $indexes = ['gender', 'hr.department'];
            $data = [
                [
                    'id'         => 1,
                    'first_name' => 'Urbano',
                    'gender'     => null,
                    'hr'         => [
                        'country'    => 'Canada',
                        'department' => 'Engineering',
                    ],
                ],
                [
                    'id'         => 2,
                    'first_name' => 'Case',
                    'gender'     => 'Male',
                    'hr'         => [
                        'country'    => null,
                        'department' => 'Marketing',
                    ],
                ],
                [
                    'id'         => 3,
                    'first_name' => 'Emera',
                    'gender'     => 'Female',
                    'hr'         => [
                        'country'    => 'France',
                        'department' => 'Engineering',
                    ],
                ],
                [
                    'id'         => 4,
                    'first_name' => 'Richy',
                    'gender'     => null,
                    'hr'         => [
                        'country'    => null,
                        'department' => 'Sales',
                    ],
                ],
                [
                    'id'         => 5,
                    'first_name' => 'Mandy',
                    'gender'     => null,
                    'hr'         => [
                        'country'    => 'France',
                        'department' => 'Sales',
                    ],
                ],
                [
                    'id'         => 6,
                    'first_name' => 'Risa',
                    'gender'     => 'Female',
                    'hr'         => [
                        'country'    => null,
                        'department' => 'Engineering',
                    ],
                ],
                [
                    'id'         => 7,
                    'first_name' => 'Alfred',
                    'gender'     => 'Male',
                    'hr'         => [
                        'country'    => 'France',
                        'department' => 'Engineering',
                    ],
                ],
                [
                    'id'         => 8,
                    'first_name' => 'Tabby',
                    'gender'     => 'Male',
                    'hr'         => [
                        'country'    => 'France',
                        'department' => 'Marketing',
                    ],
                ],
                [
                    'id'         => 9,
                    'first_name' => 'Ario',
                    'gender'     => 'Male',
                    'hr'         => [
                        'country'    => null,
                        'department' => 'Sales',
                    ],
                ],
                [
                    'id'         => 10,
                    'first_name' => 'Somerset',
                    'gender'     => 'Male',
                    'hr'         => [
                        'country'    => 'Germany',
                        'department' => 'Marketing',
                    ],
                ],
            ];
            $actual = array_group_by($data, $indexes, true);
        });

        $htmlTable = $iterator->run(3000);

        echo memory_get_peak_usage() - $memory_base;
        echo $htmlTable; exit;
    }

@datamweb
Copy link
Contributor

My quick benchmark on macOS shows recursive is slower, but the memory usage are the same.

Win 11

image

@kenjis
Copy link
Member

kenjis commented Apr 24, 2023

@datamweb It is meaningless unless both (reference and recursive) results are compared in the same environment.

@datamweb
Copy link
Contributor

@kenjis thanks, Can you suggest a link or reference for a better understanding of this issue?

@kenjis
Copy link
Member

kenjis commented Apr 24, 2023

@kenjis
Copy link
Member

kenjis commented Apr 24, 2023

@datamweb What is this issue?

If we are talking about benchmarks, it means that they are meaningless unless we compare the results of running them in the same environment. The faster the machine, the faster the results.

What we are discussing now in this GitHub issue is whether reference or recursive code is better on
readability, performance.
For me, recursion seems to be a little slower, but it's not that different, so recursion is fine.

The PHPStan error is a bug in PHPStan; PHPStan does not seem to understand that references change values.

@kenjis
Copy link
Member

kenjis commented Apr 26, 2023

Thanks, but there is "Others" section.
Please move to "Helpers and Functions", and use :php:func: to link to the function page.

--- a/user_guide_src/source/changelogs/v4.4.0.rst
+++ b/user_guide_src/source/changelogs/v4.4.0.rst
@@ -87,6 +87,9 @@ Libraries
 Helpers and Functions
 =====================
 
+- **Array:** Added :php:func:`array_group_by()` helper function to group data
+  values together. Supports dot-notation syntax.
+
 Others
 ======
 
@@ -103,7 +106,6 @@ Others
 - **Request:** Added ``IncomingRequest::setValidLocales()`` method to set valid locales.
 - **Table:** Added ``Table::setSyncRowsWithHeading()`` method to synchronize row columns with headings. See :ref:`table-sync-rows-with-headings` for details.
 - **Error Handling:** Now you can use :ref:`custom-exception-handlers`.
-- **Array:** Added ``array_group_by()`` helper function to group data values together. Supports dot-notation syntax.
 
 Message Changes
 ***************

Copy link
Member

@kenjis kenjis left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM!

@kenjis kenjis merged commit 3089cda into codeigniter4:4.4 May 14, 2023
@rumpfc rumpfc deleted the feature-array-group-by branch August 23, 2023 16:29
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
4.4 enhancement PRs that improve existing functionalities
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants